Skip to content

Don't omit "/" if it has QUERY_STRING.#5

Open
hiratara wants to merge 1 commit intolibwww-perl:masterfrom
hiratara:rfc1738
Open

Don't omit "/" if it has QUERY_STRING.#5
hiratara wants to merge 1 commit intolibwww-perl:masterfrom
hiratara:rfc1738

Conversation

@hiratara
Copy link
Contributor

@hiratara hiratara commented Jan 27, 2012

RFC 1738 says, `If neither <path> nor <searchpart> is present, the "/" may also be omitted.'.

http://www.rfc-editor.org/rfc/rfc1738.txt

So we can't omit "/" if the URL has '?' and QUERY_STRINGs.

I thought that the following patch is better than this pull-req, but it makes t/old-base.t broken.

diff --git a/URI/http.pm b/URI/http.pm
index cb69822..8631e40 100644
--- a/URI/http.pm
+++ b/URI/http.pm
@@ -13,7 +13,7 @@ sub canonical
     my $other = $self->SUPER::canonical;

     my $slash_path = defined($other->authority) &&
-        !length($other->path) && !defined($other->query);
+        !length($other->path) && defined($other->query);

     if ($slash_path) {
        $other = $other->clone if $other == $self;
diff --git a/t/http.t b/t/http.t
index 2b8c44b..65e32cd 100644
--- a/t/http.t
+++ b/t/http.t
@@ -1,6 +1,6 @@
 #!perl -w

-print "1..15\n";
+print "1..17\n";

 use URI;

@@ -61,3 +61,12 @@ $u = URI->new("http://%77%77%77%2e%70%65%72%6c%2e%63%6f%6d/%7
 print "not " unless $u->canonical eq "http://www.perl.com/pub/a/2001/08/27/bjor
 print "ok 15\n";

+# RFC 1738: "/" can't be ommited when <searchpart> is present.
+$u = URI->new("http://localhost?p=1");
+print "not " unless $u->canonical eq "http://localhost/?p=1";
+print "ok 16\n";
+
+$u = URI->new("http://localhost");
+print "not " unless $u->canonical eq "http://localhost";
+print "ok 17\n";
+

@hiratara
Respect RFC 1738.
928222a 
We can also omit "/" when <path> and <searchpart> are empty,
but it makes t/old-base.t fail.
@gisle
Copy link
Member

gisle commented Feb 14, 2012

RFC 2396 does not agree on this AFAICT.

@hiratara
Copy link
Contributor Author

hiratara commented Mar 26, 2012

RFC3986 says that RFC2396 was obsolete. So we should follow RFC3986(and RFC1738), shouldn't we ??

http://www.ietf.org/rfc/rfc3986.txt

@gisle
Copy link
Member

gisle commented Mar 30, 2012

RFC3986:

URI           = scheme ":" hier-part [ "?" query ] [ "#" fragment ]
hier-part     = "//" authority path-abempty
path-abempty  = *( "/" segment )

This does allow "http://localhost?p=1". Does compatiblity with other stuff break with this, or do you want to argue the aesthetics of of this as canonical form?

@gisle
Copy link
Member

gisle commented Mar 30, 2012

We still have this inconsistency:

$ perl -MURI -le 'print URI->new("http://foo#bar")->canonical'
http://foo/#bar
$ perl -MURI -le 'print URI->new("http://foo?bar")->canonical'
http://foo?bar

It's better to be consistent, so one of these ought to change.

@oalders oalders requested a review from Copilot August 20, 2025 22:07
Copilot AI reviewed Aug 20, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the canonical URL formatting to comply with RFC 1738, which requires the "/" path separator to be retained when a query string is present. The change ensures that URLs like "http://localhost?p=1" are canonicalized to "http://localhost/?p=1" instead of omitting the slash.

  • Modifies the canonical method logic to always add "/" when path is empty, regardless of query presence
  • Adds a test case to verify the fix works correctly for URLs with query strings

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
URI/http.pm Simplified slash_path condition to always add "/" when path is empty and authority is defined
t/http.t Added test case for URL canonicalization with query string and updated test count

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

t/http.t
print "not " unless $u->canonical eq "http://www.perl.com/pub/a/2001/08/27/bjornstad.html";
print "ok 15\n";

# RFC 1738: "/" can't be ommited when <searchpart> is present.
Copy link

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a typo in the comment: 'ommited' should be 'omitted'.

Suggested change
# RFC 1738: "/" can't be ommited when <searchpart> is present.
# RFC 1738: "/" can't be omitted when <searchpart> is present.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hiratara @gisle